Skip to content

Modify Alertmanager config metrics #3289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Oct 14, 2020
Merged

Modify Alertmanager config metrics #3289

merged 7 commits into from
Oct 14, 2020

Conversation

@jpdstan
Copy link
Contributor

@jpdstan jpdstan commented Oct 6, 2020 •
edited
Loading

Signed-off-by: Stan Kwong [email protected]

What this PR does:

  • Changes cortex_alertmanager_config_invalid to cortex_alertmanager_config_last_reload_successful, such that the nomenclature matches that of the ruler and the upstream alertmanager
  • Adds the alertmanager_config_last_reload_successful_seconds metric

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci
pracucci approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change LGTM, but I would like to have @gotjosh and/or @gouthamve checking it since they recently worked on that (in particular, I would like to know if we're OK with this change).

@jpdstan jpdstan force-pushed the add_am_metrics branch 2 times, most recently from 3c52172 to 3c880db Compare October 12, 2020 21:34
@jpdstan
Copy link
Contributor Author

jpdstan commented Oct 13, 2020

Any idea why my integration tests keep timing out?

@pracucci
Copy link
Contributor

Any idea why my integration tests keep timing out?

I will look into it. Do not worry too much about it. We'll take care of that.

gotjosh
gotjosh suggested changes Oct 13, 2020
View reviewed changes
@pracucci pracucci self-requested a review October 13, 2020 10:03
@pracucci
Copy link
Contributor

I've just merged the PR #3335 to increase integration tests timeout. Could you rebase this PR, please?

@jpdstan
Modify Alertmanager config metrics
8a54908 
Signed-off-by: Stan Kwong <[email protected]>
@jpdstan
Fix gauge
3312883 
Signed-off-by: Stan Kwong <[email protected]>
@jpdstan
Fix tests
67405fa 
Signed-off-by: Stan Kwong <[email protected]>
@jpdstan
docs: add to CHANGELOG
0fb014f 
Signed-off-by: Stan Kwong <[email protected]>
pracucci
pracucci approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@jpdstan
Copy link
Contributor Author

jpdstan commented Oct 13, 2020

@pracucci still timing out :(

@pstibrany
Copy link
Contributor

There is an alertmanager_test.go in the integration tests which is still using the old metric name.

@jpdstan
Copy link
Contributor Author

jpdstan commented Oct 14, 2020 •
edited
Loading

9bbc5c9 @pstibrany good catch!! too bad those timeouts don't trigger a test failure. took the opportunity to add my timestamp metric in those tests as well. thanks for your help!

pracucci
pracucci reviewed Oct 14, 2020
View reviewed changes
@pracucci pracucci merged commit b135907 into cortexproject:master Oct 14, 2020
@jpdstan jpdstan deleted the add_am_metrics branch October 14, 2020 18:25
@gotjosh gotjosh mentioned this pull request Nov 18, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@pracucci pracucci pracucci approved these changes

@gotjosh gotjosh gotjosh requested changes

@pstibrany pstibrany pstibrany approved these changes

@gouthamve gouthamve gouthamve approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jpdstan @pracucci @pstibrany @gotjosh @gouthamve